Skip to content

Conversation

@chr1831
Copy link

@chr1831 chr1831 commented Mar 29, 2013

Gitlab-Shell currently has the path and server for Redis hard-coded within lib/gitlab_update.rb.

With this patch users will be able to modify the redis settings within config.yml to successfully display commit notices on their Dashboards since gitlab will pickup the commit notices from Sidekiq/Redis.

Offending line causing a headache as git commits were not being posted to the user and project dashboard:
command = "env -i redis-cli rpush 'resque:gitlab:queue:post_receive' '{\"class\":\"PostReceive\",\"args\":[\"#{@repo_path}\",\"#{@oldrev}\",\"#{@newrev}\",\"#{@refname}\",\"#{@key_id}\"]}' > /dev/null 2>&1"

I've included the option to control the namespace, because it must be specified in gitlab's resque.yml + environments/.yml under cache as *:redis_store, "redis://redis.example.com:port/namespace**

Validated against ruby v1.9.3-p392 and rspec v2.12.0

@coveralls
Copy link

Coverage remained the same when pulling 93bfff7 on chr1831:master into ff484e6 on gitlabhq:master.

View Details

@coveralls
Copy link

Coverage remained the same when pulling 8c55d8f on chr1831:master into ff484e6 on gitlabhq:master.

View Details

@drf
Copy link

drf commented Apr 7, 2013

++, although this patch doesn't cover the case where you want gitlab-shell to connect to redis via a unix socket (redis-cli -s /var/run/redis/redis.sock). Would it be possible to support that case as well? Thanks for the patch!

@chr1831
Copy link
Author

chr1831 commented Apr 8, 2013

I'll include the socket parameter later tonight or tomorrow after I get my dev box backup.

@drf
Copy link

drf commented Apr 8, 2013

I really appreciate this. Thanks!

@chr1831
Copy link
Author

chr1831 commented Apr 10, 2013

It took a bit longer then I expected to get KVM running on my dev box. I am adding the patch right now. Hopefully you can test it since I don't have a local redis install on the same VM.

Commit should be added within the next 20 or so minutes.

@coveralls
Copy link

Coverage decreased (-0.3%) when pulling 93549d9 on chr1831:master into ff484e6 on gitlabhq:master.

View Details

@coveralls
Copy link

Coverage decreased (-0.3%) when pulling dec440f on chr1831:master into ff484e6 on gitlabhq:master.

View Details

@chr1831
Copy link
Author

chr1831 commented Apr 10, 2013

Whoops, I forgot to update the config.yml.example with the socket configuration option. Updated repository with the amended config.yml.example.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small bug in this version: "rpush '#{@redis['namespace']}:queue:post_receive'" should go into command instead of redis_command. Besides the code duplication, this also causes default configuration (redis.empty? == true) not to take in that command.

@coveralls
Copy link

Coverage decreased (-0.3%) when pulling 2d98c91 on chr1831:master into ff484e6 on gitlabhq:master.

View Details

@chr1831
Copy link
Author

chr1831 commented Apr 10, 2013

Per your request, I've modified the update hook and fixed the typo. I've also refactored the initialize class to initialize only one copy of GitlabConfig instead of two.

On a site note coding via nano is quite a pain. I should have caught that myself, but my dev vm with my full IDE is offline still.

@drf
Copy link

drf commented Apr 10, 2013

Looks great now, I just applied it to my installation. Thanks :) @randx , could you please have a look?

@jirutka
Copy link
Contributor

jirutka commented Apr 16, 2013

+1

@j23d
Copy link

j23d commented Apr 17, 2013

Applied this to my gitlab 5.0 installation and it fixed the issue that commits was not posted to the dashboards.

@dzaporozhets
Copy link
Contributor

Looks good for me. thank you

dzaporozhets added a commit that referenced this pull request Apr 19, 2013
Added configuration options for custom redis servers as well as bin locations
@dzaporozhets dzaporozhets merged commit 859fd98 into gitlabhq:master Apr 19, 2013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If @redis.empty? and we fallback to old connecting we got wrong behaviour here because @redis['namespace'] is nil

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll submit a new pull request later this evening that will switch out the command used based on @redis existing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by c110d11

zzet pushed a commit to zzet/gitlab-shell that referenced this pull request Jun 26, 2014
dzaporozhets added a commit that referenced this pull request Nov 25, 2015
Revert "Run git-lfs-authenticate script with original command line arguments"

This reverts commit 8449979.

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Related to: #35, gitlab/gitlabhq#2635

See merge request !68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants